feat: Support multiple MOFED DS #691

rollandf · 2023-11-22T13:47:23Z

Mofed driver precompiled container images
are compiled using a specific Kernel.

As a result, the Mofed Driver DaemonSet should
have the Kernel as part of the Node Selector.

In addition, since there can be Nodes with different Kernel versions, a DaemonSet for each existing Kernel in the cluster is created.

rollandf · 2023-11-30T07:49:50Z

/retest-all

rollandf · 2023-12-19T08:23:55Z

/retest-all

rollandf · 2024-01-01T09:01:01Z

/retest-all

rollandf · 2024-01-01T10:17:33Z

/retest-blackduck_scan

rollandf · 2024-01-01T16:09:32Z

/retest-blackduck_scan

rollandf · 2024-01-03T21:13:47Z

Depends on NVIDIA/k8s-operator-libs#8

killianmuldoon

General question - in the default case for someone updating to Network Operator 24.04 this will cause a full rollout of the DS across all nodes, even if the version of OFED etc. remains the same. The only way to avoid this AFAICT is to opt out of migration using an environment variable.

Is this the expected upgrade path for users in general? i.e. a network operator update requires a rollout of OFED - meaning nodes are offline for a non-trivial period?

pkg/nodeinfo/attributes.go

pkg/nodeinfo/node_info.go

killianmuldoon · 2024-03-04T09:11:13Z

pkg/utils/utils.go

+func GetStringHash(s string) string {
+	hasher := fnv.New32a()
+	if _, err := hasher.Write([]byte(s)); err != nil {
+		panic(err)
+	}
+	return rand.SafeEncodeString(fmt.Sprint(hasher.Sum32()))
+}


We should probably just keep this private and beside where it's used unless there's a reason to have it in this Utils package

Agreed, moved to state package

pkg/utils/utils_test.go

go.mod

killianmuldoon · 2024-03-04T09:35:48Z

main.go

-	if err := migrate.Migrate(stopCtx, setupLog.WithName("migrate"), directClient); err != nil {
-		setupLog.Error(err, "failed to run migration logic")
-		os.Exit(1)
+	migrationCompletionChan := make(chan struct{})
+	m := migrate.Migrator{
+		K8sClient:      directClient,
+		MigrationCh:    migrationCompletionChan,
+		LeaderElection: enableLeaderElection,
+		Logger:         ctrl.Log.WithName("Migrator"),
 	}


What's the reason for moving this logic to a runnable?

We want to use the leader election mechanism and make sure it runs only when we get the leader election by using mgr.Add(&m) which requires a runnable.
Otherwise old instance of operator can recreate the DS after the new one deleted.
In Helm, we have a pre-hook that scale down the deployment, but we won't have it in OpenShift

Would it be possible to deprecate the helm hooks in favor of this approach?

I think one of thing we would like is to get rid of creating the instance of NicClusterPolicy as part of the helm before we go and remove this hook.

The process should be:

upgrade operator

wait for operator to be running

update NCP with new values

Let's try to get this prioritized IMO - makes a lot of things simpler for deploying network operator.

rollandf · 2024-03-04T16:23:00Z

General question - in the default case for someone updating to Network Operator 24.04 this will cause a full rollout of the DS across all nodes, even if the version of OFED etc. remains the same. The only way to avoid this AFAICT is to opt out of migration using an environment variable.

Is this the expected upgrade path for users in general? i.e. a network operator update requires a rollout of OFED - meaning nodes are offline for a non-trivial period?

Usually, Network Operator release comes with a new MOFED, meaning a rollout of nodes during the upgrade.
For one time, when this feature is out, if user use same MOFED as before an upgrade will still happens, we could not find another option.
The env variable is not a real option for end users, this is just an escape plan if things go wrong. We want all users to migrate to the multiple DS to support pre-compiled MOFED.

killianmuldoon

A couple of smaller comments - but looks generally good to me

killianmuldoon · 2024-03-06T10:41:06Z

pkg/state/state_ofed.go

@@ -504,7 +516,7 @@ func (s *stateOFED) getInitContainerConfig(
 // getMofedDriverImageName generates MOFED driver image name based on the driver version specified in CR
 // TODO(adrianc): in Network-Operator v1.5.0, we should just use the new naming scheme


Can we remove this TODO? Not sure exactly what it's refereeing to.

killianmuldoon · 2024-03-06T10:43:50Z

pkg/state/state_ofed.go

 		reqLogger.V(consts.LogLevelInfo).Info("No nodes with Mellanox NICs where found in the cluster.")
 		return []*unstructured.Unstructured{}, nil
 	}
+	objs := make([]*unstructured.Unstructured, 0)
+	renderedObjsMap := stateObjects{}
+	for _, np := range nodePools {


Could we break out what's inside this loop into a seperate function?

killianmuldoon · 2024-03-06T10:50:01Z

pkg/state/state_ofed.go

+	for _, np := range nodePools {
+		nodePool := np
+
+		useDtk := clusterInfo.IsOpenshift() && config.FromEnv().State.OFEDState.UseDTK


I think some of the actions in this loop - setting defaults on the NCP, checking if we useDTK, certconfig and repo config can be handled outside this loop and just passed into render the manifest when we need it. WDYT?

Done.
certconfig and repo config need to be in the loop it needs osname, that in theory can different in pools

killianmuldoon · 2024-03-06T11:05:33Z

controllers/upgrade_controller.go

+	select {
+	case <-r.MigrationCh:
+	case <-ctx.Done():
+		return ctrl.Result{}, fmt.Errorf("canceled")
+	}


We should probably add this to all controllers - even if not strictly needed right now. Might even be worth doing a reconcile-wrapper, but I think this snipped as is in all controllers is good enough for now. Maybe also add a comment to give folks clues as to what we're waiting for at this point.

nit: it would be good to make it in a separate commit

ykulazhenkov

I checked the PR. It looks good. LGTM after rebase and addressing Killian's comments.

e0ne

LGTM once it will be rebased

killianmuldoon

Just some nits around logging - maybe worth taking as a follow-up. One log that's wrong and elsewhere we shouldn't be logging if we're returning the error.

If we want to add context we should wrap the error but let controller runtime do the logging so it's not confusing and duplicated for users.

killianmuldoon · 2024-03-11T14:44:28Z

pkg/migrate/migrate.go

@@ -55,6 +84,11 @@ func migrate(ctx context.Context, log logr.Logger, c client.Client) error {
 		log.V(consts.LogLevelError).Error(err, "error trying to remove state label on NV IPAM configmap")
 		return err
 	}
+	if err := handleSingleMofedDS(ctx, log, c); err != nil {
+		// critical for the operator operation, will fail Mofed migration
+		log.V(consts.LogLevelError).Error(err, "error trying to remove state label on NV IPAM configmap")


This error message should be updated

killianmuldoon · 2024-03-11T14:53:26Z

pkg/migrate/migrate.go

+		log.V(consts.LogLevelDebug).Info("NIC ClusterPolicy not found, skip handling single MOFED DS")
+		return nil
+	} else if err != nil {
+		log.V(consts.LogLevelError).Error(err, "fail to get NIC ClusterPolicy")


No need to log if we're returning the error here AFAIK.

Mofed driver precompiled container images are compiled using a specific Kernel. As a result, the Mofed Driver DaemonSet should have the Kernel as part of the Node Selector. In addition, since there can be Nodes with different Kernel versions, a DaemonSet for each existing Kernel in the cluster is created. In the Migration module, the former DS is deleted with DeletePropagationOrphan so that MOFED pods will still exists until manual or auto-upgrade is done. Signed-off-by: Fred Rolland <[email protected]>

killianmuldoon

/lgtm

/approve

ykulazhenkov

LGTM

e0ne · 2024-03-12T08:19:30Z

controllers/upgrade_controller.go

+	select {
+	case <-r.MigrationCh:
+	case <-ctx.Done():
+		return ctrl.Result{}, fmt.Errorf("canceled")
+	}


nit: it would be good to make it in a separate commit

rollandf added the on hold This enhancement is currently on hold pending additional clarification and evaluation label Nov 22, 2023

rollandf force-pushed the nodepool branch from 0703b63 to c2227fe Compare November 22, 2023 14:19

rollandf force-pushed the nodepool branch 7 times, most recently from e765759 to f1602c3 Compare December 18, 2023 17:03

rollandf force-pushed the nodepool branch 2 times, most recently from 329b06a to 57a1c3d Compare December 31, 2023 20:17

rollandf mentioned this pull request Jan 1, 2024

feat: Support ForcePrecompiled flag #727

Merged

rollandf force-pushed the nodepool branch 2 times, most recently from f1501b6 to 7556451 Compare January 3, 2024 21:13

rollandf force-pushed the nodepool branch 2 times, most recently from b3d392c to bf02b16 Compare January 11, 2024 08:51

rollandf force-pushed the nodepool branch 2 times, most recently from ff1ffc2 to b635fad Compare March 3, 2024 09:44

rollandf removed the on hold This enhancement is currently on hold pending additional clarification and evaluation label Mar 3, 2024

rollandf requested review from ykulazhenkov, adrianchiris and e0ne March 3, 2024 11:37

killianmuldoon reviewed Mar 4, 2024

View reviewed changes

rollandf force-pushed the nodepool branch from b635fad to 429c54a Compare March 4, 2024 16:01

rollandf force-pushed the nodepool branch from 429c54a to f8e2105 Compare March 4, 2024 16:09

killianmuldoon reviewed Mar 6, 2024

View reviewed changes

ykulazhenkov reviewed Mar 6, 2024

View reviewed changes

e0ne reviewed Mar 6, 2024

View reviewed changes

rollandf force-pushed the nodepool branch 2 times, most recently from cde2088 to 7d70631 Compare March 7, 2024 12:31

ykulazhenkov previously approved these changes Mar 11, 2024

View reviewed changes

killianmuldoon reviewed Mar 11, 2024

View reviewed changes

rollandf dismissed ykulazhenkov’s stale review via 600649f March 11, 2024 15:41

rollandf force-pushed the nodepool branch from 7d70631 to 600649f Compare March 11, 2024 15:41

rollandf force-pushed the nodepool branch from 600649f to a8f1a6e Compare March 11, 2024 15:48

killianmuldoon reviewed Mar 11, 2024

View reviewed changes

killianmuldoon approved these changes Mar 11, 2024

View reviewed changes

ykulazhenkov approved these changes Mar 12, 2024

View reviewed changes

e0ne approved these changes Mar 12, 2024

View reviewed changes

rollandf merged commit 7193b1e into Mellanox:master Mar 13, 2024
16 checks passed

rollandf deleted the nodepool branch August 27, 2024 08:55

		@@ -504,7 +516,7 @@ func (s *stateOFED) getInitContainerConfig(
		// getMofedDriverImageName generates MOFED driver image name based on the driver version specified in CR
		// TODO(adrianc): in Network-Operator v1.5.0, we should just use the new naming scheme

feat: Support multiple MOFED DS #691

feat: Support multiple MOFED DS #691

Conversation

rollandf commented Nov 22, 2023

rollandf commented Nov 30, 2023

rollandf commented Dec 19, 2023

rollandf commented Jan 1, 2024

rollandf commented Jan 1, 2024

rollandf commented Jan 1, 2024

rollandf commented Jan 3, 2024

killianmuldoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rollandf commented Mar 4, 2024

killianmuldoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ykulazhenkov left a comment

Choose a reason for hiding this comment

e0ne left a comment

Choose a reason for hiding this comment

killianmuldoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killianmuldoon left a comment

Choose a reason for hiding this comment

ykulazhenkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment